Expose bind leader directly #4836
Conversation
christophermaier
requested review from
baumanj,
cnunciato,
fnichol,
mgamini and
reset
as code owners
|
Thanks for the pull request! Here is what will happen next:
Thank you for contributing! |
| @@ -478,8 +478,17 @@ | |||
| "properties": { | |||
| "first": { | |||
| "description": "The first member of this service group. If the group is running in a leader topology, this will also be the leader.", | |||
| "$deprecated": "Since 0.56.0; if you want the leader, use `leader` explicitly. 'first' isn't deterministic, either, so you can just use `members[0]` instead", | |||
There was a problem hiding this comment.
I'm not up on the whole handlebars world, but I remember seeing something about the syntax changing to something like members.[0]. Does that apply here?
There was a problem hiding this comment.
@baumanj Habitat's current syntax is foo[0], but the Handlebars library we use recently fixed itself to use the proper foo.[0] syntax. We can't upgrade our Handlebars library right now, though, because it would break any template that has the foo[0] syntax. So, until then, we have to keep the old way around 😦
|
|
|
hey @christophermaier I'm thinking we should include a generic example in the documentation, like what's in the If I'm interpreting this PR correctly, would this be the right syntax? right? |
|
@baumanj Manos: The Templates of Fate |
|
@irvingpop Yup, you've got the idea 👍 |
|
After sleeping on this, I'm wondering if we could use this opportunity to improve the UX a bit - since this if/else statement will wind up in everyone's plans - would you consider some ideas to make it more terse and expressive? some ideas in no particular order:
some users may also prefer going the other way and wanting more control:
|
|
Just to make things visible... @irvingpop and I will be chatting more about ideas for improving template data next week. It's fine for this PR to stay open until at least then. |
|
Had a chat with @irvingpop and captured ideas for the future in ##4872. Nothing affects the correctness or suitability of this PR, so it's open again to comment and merging. |
There was a problem hiding this comment.
I'm loving what you are doing with the rendering context schema. These improvements are huge. Also yes to bind.{x}.leader.
It's worth mentioning to any reader who is unaware, the reason we had so much duplicated data was because we first started with mustache as our rendering engine and later changed to handlebars. These changes have been a long time coming since that switch (back in 2016 😬)
christophermaier
force-pushed
the
cm/4127-expose-bind-leader-directly
branch
from
1536148
to
16f6009
Compare
Signed-off-by: Christopher Maier <[email protected]>
Signed-off-by: Christopher Maier <[email protected]>
Previously, determining the leader of a service group was
complicated. If the service was running in a leader topology, the
`{{bind.<BIND>.first}}` key would be the leader, but we didn't
directly indicate if that was the case (you could have queried to see
if `{{bind.<BIND>.first.leader}}` was `true`, but that leads to some
needlessly verbose templates.
Now, we introduce an explicit `leader` key, which will be non-null if
the service group has a leader. Furthermore, the use of `first` is
deprecated, since the meaning of it is not deterministic (it could be
a non-alive member of the service group, for instance, which is not
what anyone wants). The `first` key will remain in rendering contexts
for the foreseeable future, but users are encouraged to use
`{{bind.<BIND>.leader}}` if they want the leader, and
`{{bind.<BIND>.members[0]}}` otherwise, pending future refactorings.
Documentation is updated to reflect this, and a new `$deprecated` and
`$since` key are used in our JSON Schema to indicate when an old field
is deprecated, and when a new one is introduced. Our JSON Schema
parser is configured to allow non-standard keywords like this through
its processing, and this provides some additional hooks that a future
automated documentation generator can use (see #4824)
Fixes #4127
Signed-off-by: Christopher Maier <[email protected]>
christophermaier
force-pushed
the
cm/4127-expose-bind-leader-directly
branch
from
16f6009
to
f2e4467
Compare
Previously, determining the leader of a service group was
complicated. If the service was running in a leader topology, the
{{bind.<BIND>.first}}key would be the leader, but we didn'tdirectly indicate if that was the case (you could have queried to see
if
{{bind.<BIND>.first.leader}}wastrue, but that leads to someneedlessly verbose templates.
Now, we introduce an explicit
leaderkey, which will be non-null ifthe service group has a leader. Furthermore, the use of
firstisdeprecated, since the meaning of it is not deterministic (it could be
a non-alive member of the service group, for instance, which is not
what anyone wants). The
firstkey will remain in rendering contextsfor the foreseeable future, but users are encouraged to use
{{bind.<BIND>.leader}}if they want the leader, and{{bind.<BIND>.members[0]}}otherwise, pending future refactorings.Documentation is updated to reflect this, and a new
$deprecatedand$sincekey are used in our JSON Schema to indicate when an old fieldis deprecated, and when a new one is introduced. Our JSON Schema
parser is configured to allow non-standard keywords like this through
its processing, and this provides some additional hooks that a future
automated documentation generator can use (see #4824)
Fixes #4127